fix(observability): add TTL-based eviction to prevent memory leak (#563) - #881
Conversation
…is-IV#563) The module-level events array in metricsStore.ts grew without time-based bounds in long-running Node.js processes. Added a purgeStaleEvents() function that removes events older than 1 hour on every ingest and snapshot read, complementing the existing MAX_EVENTS size cap.
|
@Seunfunmi-319509 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Auto-review failed (API error). Leaving PR for human review. |
|
Auto-review failed (API error). Leaving PR for human review. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const parts = fullKey.split("."); | ||
| const key = parts[parts.length - 1]; | ||
| const namespace = parts.length > 1 ? parts[0] : ''; | ||
| const namespace = parts.length > 1 ? parts[0] : ""; |
…MESSAGE error in NotificationBell/NotificationSettings
|
Auto-review failed (API error). Leaving PR for human review. |
davidmaronio
left a comment
davidmaronio
left a comment
There was a problem hiding this comment.
the core metricsStore change is solid: purgeStaleEvents correctly exploits insertion order with findIndex plus splice, clears the whole buffer when everything is stale, and purging on both ingest and snapshot reads keeps the TTL and the existing MAX_EVENTS cap complementary. good, focused design for the actual fix.
the problem is the rest of the diff. the PR touches ~20 unrelated files (README, i18n scripts, WalletContext, DonationContext, contractClient, CampaignMap, e2e specs, etc). your branch was cut from the broken main state that #875 later fixed, so those hunks would re-revert other people's work if merged as is.
- please rebase onto current main so the diff only contains src/lib/observability/metricsStore.ts (plus a test). everything outside observability should disappear from the diff.
- src/lib/observability/metricsStore.ts: there is no unit test for the new TTL behavior. please add one covering the three purge cases (some stale at front, all stale, none stale) using a fake now.
- heads up on merge order: #767 (rate limiting) is expected to land first and #784 (auth) also touches this observability surface. after those merge you will need to re-check that ingest still calls purgeStaleEvents exactly once per event and that rate-limit counters are not fed from purged events.
ci is red mostly because of the stale base, so a clean rebase should also fix lint and playwright.
4 participants
The module-level events array in metricsStore.ts grew without time-based bounds in long-running Node.js processes. Added a purgeStaleEvents() function that removes events older than 1 hour on every ingest and snapshot read, complementing the existing MAX_EVENTS size cap.
Summary
Closes #563
Type of Change
Contributor Checklist
CONTRIBUTING.mdfor branch, commit, and PR title conventions.Validation
npm run lintnpm run format:checknpm run typechecknpm testnpm run buildNotes for Reviewers